feat: bound incoming request and add postgres service #76
Conversation
|
I've assigned @tankyleo as a reviewer! |
enigbe
force-pushed
the
2025-12-bound-incoming-request-and-postgres-service
branch
from
4bfb403 to
a163ae7
Compare
rust/server/src/vss_service.rs
Outdated
| use std::pin::Pin; | ||
| use std::sync::Arc; | ||
|
|
||
| const MAXIMUM_REQUEST_BODY_SIZE: u16 = 65_535; |
There was a problem hiding this comment.
This seems very conservative, given that monitors could get quite large. Given that the VSS service is actually a storage service, it also might make sense to make this configurable (in contrast to lightningdevkit/ldk-server#80, but even there we set the limit to 10MB).
There was a problem hiding this comment.
As mentioned elsewhere: I guess a static upper bound is a good first step, but if we're really concerned about DoS we might need some dynamic rate limiting on a per-IP basis. Although then the question becomes how much of that should be considered the concern of the VSS service itself, and how much we'd just expect users to slap a load balancer/Cloudflare in front of the service to handle that for them
There was a problem hiding this comment.
I've made the configuration changes suggested here, capping at 20 MB for the maximum size.
There was a problem hiding this comment.
No, as mentioned elsewhere, individual values might be quite a bit larger than 10 or 20MB. I think something along the lines of 100MB would be a more reasonable maximum, though per request it raises the question of how much a DoS protection this actually is.
enigbe
force-pushed
the
2025-12-bound-incoming-request-and-postgres-service
branch
from
5d391cc to
fbdd957
Compare
|
🔔 1st Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 7th Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 8th Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 9th Reminder Hey @tankyleo! This PR has been waiting for your review. |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Can we also add some test coverage to ensure the database / backends actually support the configured maximum values, i.e., that we're not otherwise limited somehow?
rust/server/src/vss_service.rs
Outdated
| use std::sync::Arc; | ||
|
|
||
| const MAXIMUM_REQUEST_BODY_SIZE: usize = 20 * 1024 * 1024; | ||
| const DEFAULT_REQUEST_BODY_SIZE: usize = 10 * 1024 * 1024; |
There was a problem hiding this comment.
Not quite sure why we have two values now? Why have DEFAULT and a separate MAXIMUM?
rust/server/src/vss_service.rs
Outdated
| use std::pin::Pin; | ||
| use std::sync::Arc; | ||
|
|
||
| const MAXIMUM_REQUEST_BODY_SIZE: u16 = 65_535; |
There was a problem hiding this comment.
No, as mentioned elsewhere, individual values might be quite a bit larger than 10 or 20MB. I think something along the lines of 100MB would be a more reasonable maximum, though per request it raises the question of how much a DoS protection this actually is.
rust/server/src/vss_service.rs
Outdated
| use std::pin::Pin; | ||
| use std::sync::Arc; | ||
|
|
||
| const MAXIMUM_REQUEST_BODY_SIZE: usize = 20 * 1024 * 1024; |
There was a problem hiding this comment.
10/20MiB is quite small. We generally expect VSS clients to be edge-nodes storing small payment histories, but if someone does use it as a routing node its easy for monitors to get into the 100s of MiB. Postgres' limit for non-"large object" storage is 1GiB, so that probably seems reasonable as the hard max request size (though we should maybe consider supporting large objects for larger monitor storage).
|
🔔 10th Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 11th Reminder Hey @tankyleo! This PR has been waiting for your review. |
Verifies that storage backends can handle the configured maximum value size (~1 GB). Also increases MAXIMUM_REQUEST_BODY_SIZE to 1 GB to align server-side validation with storage capacity.
enigbe
force-pushed
the
2025-12-bound-incoming-request-and-postgres-service
branch
from
5a264a6 to
97b6b25
Compare
4 participants
What this PR does
TODOmaximum_request_body_sizein [server_config]